Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Linear Map for Cargo 1.71 #376

Closed
wants to merge 16 commits into from

Conversation

AdinAck
Copy link

@AdinAck AdinAck commented Aug 31, 2023

Motivation

In the latest version of Cargo the implementation of Drop for linear map does not compile.

Changes

Revert Drop impl to previous implementation.

@birkenfeld
Copy link
Contributor

birkenfeld commented Aug 31, 2023

This patch isn't correct. Like the new code says, Vec already implements Drop, so your change will call drop() twice on the contents. Look at this example:

struct S;

impl Drop for S {
    fn drop(&mut self) {
        println!("dropped an S");
    }
}

fn main() {
    let mut v = heapless::LinearMap::<&str, S, 5>::new();
    v.insert("a", S).ok().unwrap();
}

It will print "dropped an S" twice with the PR.

Just removing the drop(&self.buffer) is enough IMO.

@AdinAck
Copy link
Author

AdinAck commented Aug 31, 2023

I did notice it continued to work when the entire Drop impl was removed, didn't realize it actually is not necessary. I will just remove it then, thanks.

@newAM
Copy link
Member

newAM commented Oct 20, 2023

There seems to be something wrong with the commits here, perhaps a git revert gone wrong?

I made a separate PR to fix in #388

@AdinAck
Copy link
Author

AdinAck commented Oct 20, 2023

cool yeah this PR desperately needed a rebase

@Dirbaio
Copy link
Member

Dirbaio commented Oct 20, 2023

fixed in #388

@Dirbaio Dirbaio closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants